Conversation
| attached[vol.GetVolumeName()] = vol | ||
|
|
||
| } | ||
|
|
There was a problem hiding this comment.
Should we skip the DB update if there are no external volumes to be attached? i.e len(attached) == 0
There was a problem hiding this comment.
I am currently doing this. In ensuredVolumesAttached, if len(mountedVols) == 0 { return actor, nil } to immediately bypass the store update. Also, I am checking !maps.Equal(vol.GetPublishContext(), pubCtx) so we also skip the DB write if the volume is already attached to that node. Answered in #1729
There was a problem hiding this comment.
Done, this will likely require a rebase.
|
|
||
| } | ||
|
|
||
| storedActor, updateErr := w.store.UpdateActor(ctx, actorRef, store.PreconditionFrom(actor), func(toUpdate *ateapipb.Actor) error { |
There was a problem hiding this comment.
How do we use this publish context information stored for a volume? I do not see a DB update on volume detach, if we plan to use the DB details to get the current publish context for the actor volume then it might not have the right details. Though, if the details are checked only for a running actor then implicitly the actor volumes are already attached and thus the DB has correct up-to-date information.
There was a problem hiding this comment.
We use PublishContext to tell the worker where the attached disk is located (i.e /dev/xvdf) so it can mount it. On detach, we now explicitly wipe it from the database (PublishContext = nil). When an actor resumes, we always attach the disk and refresh this in the DB before launching the container. Answered in #1729
There was a problem hiding this comment.
Yeah as Anish said, publish context is used during attach. Refering to Anish's PR to handle this.
There was a problem hiding this comment.
PublishContext is only referred to after an actor resume (when mounting a volume). So whenever a volume is mounted we can assume that first, the the publish context was updated for the volume.
I think clearing this is an unnecessary write.
| // +k8s:optional | ||
| // +k8s:maxProperties=32 | ||
| // +k8s:eachKey=+k8s:maxLength=128 | ||
| // +k8s:eachVal=+k8s:maxLength=256 |
There was a problem hiding this comment.
Is this a standard limitation? Can publish context frequently cross this limit, what happens if publish context > 256 characters?
There was a problem hiding this comment.
While standard device paths are generally pretty short ( <50 chars), certain drivers can obviously pass longer connection strings. To provide more headroom, I did increase it from 256 to 1024 (which should be like 10x headroom for any driver). Answered in #1729
There was a problem hiding this comment.
CSI spec's only limit is that publishContext must be less than 4KiB. Substrate requires that we have limits for number of entries, key and value though.
Right now Filestore has no publish context which is the primary driver that we are supporting. For other CSI drivers I think it's better to have a restrictive policy that we can easily loosen later if need be than having a policy that is too flexible.
Bowei Du (bowei)
added
kind/feature
Some CSI drivers require attachment metadata (e.g. device path) for publishing a volume. This PR adds a publishContext to actor volumes and persists it during a volume's lifecycle.
ee56565 to
2a468d1
Compare
4 participants
#1077
When publishing volumes, some CSI drivers require additional metadata (e.g. device) path for attachment. This PR adds a publishContext to actor external volumes and integrates into the actor life cycle.